Implementation of pygame.(F)Rect.relcenter
#3089
Conversation
|
I'm not sure about the x and y variants, as |
damusss
added
New API
|
It seems to me like most uses would be related to |
With the argument you gave with damus, indeed |
|
I agree that the feature is logical and should probably be implemented, but I wonder if there are more use cases than the one given:
If I understand this case correctly, one could use |
Starbuck5
changed the title
pygame.(F)Rect.rel_centerpygame.(F)Rect.relcenter
📝 WalkthroughTip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/reST/ref/rect.rst (1)
61-63: Docs correctness: mention that assigning relcenter resizes (it’s an exception to “all other assignments move”).Currently the paragraph says only size/width/height resize; with relcenter added, that statement is no longer accurate. Also, minor style nits in the example and rounding wording.
Apply these doc tweaks:
- Assigning to size, width or height changes the dimensions of the rectangle; - all other assignments move the rectangle without resizing it. Notice that + Assigning to ``size``, ``width``, ``height``, or ``relcenter`` changes the + dimensions of the rectangle. Assigning to ``relcenter`` resizes the rect + around its top-left (the top-left remains unchanged). All other assignments + move the rectangle without resizing it. Notice that some attributes are integers and others are pairs of integers.And in the versionadded block:
- .. code-block:: python - - > my_rect = pygame.Rect(0, 0, 2, 2) - > my_rect.relcenter - > (1, 1) - > my_rect.relcenter = (128, 128) - > my_rect.relcenter, my_rect.size - > ((128, 128), (256, 256)) + .. code-block:: python + + >>> my_rect = pygame.Rect(0, 0, 2, 2) + >>> my_rect.relcenter + (1, 1) + >>> my_rect.relcenter = (128, 128) + >>> (my_rect.relcenter, my_rect.size) + ((128, 128), (256, 256))And clarify rounding precisely (Rect truncates toward zero):
- Beware of non integer relative centers ! Using a Rect instead of FRect will round down - the values of the returned ``Point``. + Beware of non-integer relative centers! With ``Rect`` (integer sizes), + halves are truncated toward zero; with ``FRect`` (float sizes), halves preserve + fractional parts.Also applies to: 96-113
🧹 Nitpick comments (4)
test/rect_test.py (3)
581-587: Fix test docstring: says “center” but the test is for relcenter.Apply:
- """Ensures the center attribute can't be deleted.""" + """Ensures the relcenter attribute can't be deleted."""
909-916: Reduce test duplication around expected relcenter calculations.A small helper would de-duplicate the repeated expected_new_relcenter math and assertions across scale/inflate variants, improving readability and maintenance.
Example helper (define near other test utilities):
def _assert_relcenter(self, r, expected): self.assertEqual(expected, r.relcenter if isinstance(r.relcenter[0], int) else tuple(expected))Then reuse:
expected_new_relcenter = (r.w, r.h * 2) self._assert_relcenter(r2, expected_new_relcenter)Also applies to: 924-931, 940-947, 955-972, 973-995, 1001-1016, 1017-1034, 1043-1051, 1052-1059, 1061-1068, 1069-1076, 1078-1085, 1138-1149, 1154-1165, 1178-1191
3111-3126: Consider adding a direct setter test for FRect.relcenter.You verify FRect relcenter reads and many transforms, but not the explicit setter. Add one case to ensure floats are preserved and topleft remains unchanged.
Suggested test:
def test_frect_relcenter_setter(self): r = FRect(1.25, 2.5, 3.5, 4.75) old_tl = r.topleft r.relcenter = (12.8, 7.6) self.assertSeqAlmostEqual5(old_tl, r.topleft) self.assertSeqAlmostEqual5((12.8, 7.6), r.relcenter) self.assertSeqAlmostEqual5((25.6, 15.2), r.size)src_c/rect_impl.h (1)
2839-2864: Minor comment label and rounding behavior note.
- The block header reads “/center/”; rename to “/* relcenter */” for clarity.
- Optional: If you want docs to be exact, note that integer division truncates toward zero (already addressed in docs suggestion).
Apply:
-/*center*/ +/* relcenter */ static PyObject * RectExport_getrelcenter(RectObject *self, void *closure) { return TupleFromTwoPrimitives(self->r.w / 2, self->r.h / 2); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
buildconfig/stubs/pygame/rect.pyi(1 hunks)docs/reST/ref/rect.rst(2 hunks)src_c/rect.c(4 hunks)src_c/rect_impl.h(4 hunks)test/rect_test.py(26 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src_c/rect.c (1)
src_c/rect_impl.h (2)
RectExport_getrelcenter(2840-2844)RectExport_setrelcenter(2846-2864)
test/rect_test.py (1)
buildconfig/stubs/pygame/rect.pyi (9)
Rect(307-307)relcenter(99-99)relcenter(101-101)x(39-39)x(41-41)y(43-43)y(45-45)scale_by(175-175)scale_by(177-177)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: x86_64
- GitHub Check: aarch64
- GitHub Check: i686
- GitHub Check: x86_64
- GitHub Check: debug_coverage (ubuntu-24.04, 3.13.5)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.14.0rc1)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.9.23)
- GitHub Check: x86
- GitHub Check: AMD64
- GitHub Check: Debian (Bookworm - 12) [armv7]
- GitHub Check: Debian (Bookworm - 12) [armv6]
- GitHub Check: Debian (Bookworm - 12) [s390x]
- GitHub Check: Debian (Bookworm - 12) [ppc64le]
- GitHub Check: msys2 (ucrt64, ucrt-x86_64)
- GitHub Check: msys2 (clang64, clang-x86_64)
- GitHub Check: msys2 (mingw64, x86_64)
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: build (ubuntu-22.04)
- GitHub Check: dev-check
🔇 Additional comments (8)
buildconfig/stubs/pygame/rect.pyi (1)
99-101: relcenter property stub looks correct and consistent.
- Naming matches existing style (center/centerx/centery).
- Types align with other position/size properties (getter returns tuple[_N, _N]; setter accepts Point).
docs/reST/ref/rect.rst (2)
50-50: Public attribute list update is good.Adding “relcenter” right after “center” mirrors the API surface and helps discoverability.
96-96: Version marker sanity check.Please confirm the correct release number for this addition (currently “2.5.6”). If this lands in a different tag, update accordingly.
src_c/rect.c (2)
136-137: C API mappings for relcenter are wired correctly.The Rect/FRect macro aliases match the template implementation names; no symbol drift spotted.
Also applies to: 255-256
692-694: Property exposure: placement and signature look good.
- relcenter follows center in both getset tables (Rect and FRect), matching docs and stubs.
- NULL doc strings consistent with other attributes.
Also applies to: 736-738
test/rect_test.py (1)
559-572: Good coverage for relcenter basic semantics.
- Verifies resize-while-anchoring topleft and round-trip readback. Solid.
src_c/rect_impl.h (2)
287-293: Template guards include relcenter—good.The new guards mirror existing center/size guards and protect both getter and setter; consistent with the template pattern.
614-617: Prototype declarations for relcenter align with template usage.Signatures match other tuple-based accessors; no ABI surprises.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
test/rect_test.py (2)
3426-3434: Prefer almost-equal for float centers to avoid flakinessUse the existing helper for r.center vs r2.center to match the rest of the FRect tests.
Apply this diff:
- self.assertEqual(r.center, r2.center) + self.assertSeqAlmostEqual5(r.center, r2.center)
3123-3126: Consider adding setter tests for FRect.relcenter and float inputs for Rect.relcenterWould close gaps by asserting:
- FRect: setting relcenter preserves topleft and sets size to 2*value (floats).
- Rect: setting relcenter with floats coerces/truncates as per other attributes.
If helpful, I can draft concise tests covering these two cases.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
docs/reST/ref/rect.rst(3 hunks)src_c/rect_impl.h(4 hunks)test/rect_test.py(26 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src_c/rect_impl.h
- docs/reST/ref/rect.rst
🧰 Additional context used
🧬 Code graph analysis (1)
test/rect_test.py (1)
buildconfig/stubs/pygame/rect.pyi (25)
Rect(307-307)left(51-51)left(53-53)top(47-47)top(49-49)relcenter(99-99)relcenter(101-101)topleft(63-63)topleft(65-65)size(111-111)size(113-113)w(123-123)w(125-125)h(127-127)h(129-129)centerx(103-103)centerx(105-105)x(39-39)x(41-41)y(43-43)y(45-45)scale_by(175-175)scale_by(177-177)centery(107-107)centery(109-109)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: x86_64
- GitHub Check: x86_64
- GitHub Check: aarch64
- GitHub Check: i686
- GitHub Check: msys2 (ucrt64, ucrt-x86_64)
- GitHub Check: msys2 (mingw64, x86_64)
- GitHub Check: msys2 (clang64, clang-x86_64)
- GitHub Check: x86
- GitHub Check: AMD64
- GitHub Check: Debian (Bookworm - 12) [armv7]
- GitHub Check: Debian (Bookworm - 12) [armv6]
- GitHub Check: Debian (Bookworm - 12) [s390x]
- GitHub Check: Debian (Bookworm - 12) [ppc64le]
- GitHub Check: build (ubuntu-22.04)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.14.0rc1)
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: build (windows-latest)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.9.23)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.13.5)
- GitHub Check: dev-check
🔇 Additional comments (25)
test/rect_test.py (25)
559-587: Relcenter basics: getter/setter, validation, and deletion look correctSetting relcenter resizes from topleft as intended; invalid types and deletion are handled consistently with other Rect attrs. Nice coverage.
866-876: LGTM: relcenter tracks inflate(-x, -y) for RectExpected relcenter uses integer halves after size change; assertions match center-preserving semantics.
898-908: LGTM: relcenter tracks inflate_ip(-x, -y) for RectCorrect integer math and invariants.
913-923: LGTM: scale_by(2) updates relcenter to previous sizeRelcenter equals (new_w//2, new_h//2) = old size; assertions are consistent.
929-939: LGTM: scale_by(x=2) single-arg kw pathSame expectations as positional; good redundancy.
944-954: LGTM: scale_by(0.5) halves relcenterInteger floor semantics captured via // for Rect.
962-972: LGTM: scale_by(2, 4) mixed scalingRelcenter expectation (w, h*2) is correct; edges/center assertions align.
985-995: LGTM: scale_by(scale_by=(2, 4)) kwargs variantCovers tuple-arg path; math checks out.
1006-1016: LGTM: scale_by(x=2, y=4) kw variantRelcenter and bounds assertions are sound.
1024-1034: LGTM: scale_by(0.5, 0.25) downscaleCorrect use of // for integer Rect semantics.
1043-1085: LGTM: subzero and negative factors handlingRelcenter expectations for large/negative scales are consistent and symmetric; good regression coverage.
1139-1149: LGTM: scale_by_ip(2) in-place doubles relcenterIn-place path mirrors functional variant; assertions consistent.
1155-1165: LGTM: scale_by_ip(0.5) in-place halves relcenterCovers integer rounding correctly.
1181-1191: LGTM: scale_by_ip(x=2, y=4) kwargsRelcenter and bounds preserved; nice coverage of kw path.
3123-3133: LGTM: FRect relcenter equals (w/2, h/2) with float semanticsBaseline expectation verified alongside center and midpoints.
3153-3164: LGTM: FRect scale_by(2.3) relcenter mathUses 1.15 factor; tolerances handled via AlmostEqual5 helper.
3170-3181: LGTM: FRect scale_by(x=2.3) kwDuplicate path validated with float assertions.
3186-3196: LGTM: FRect scale_by(0.5)Expected relcenter fractions and bounds deltas look right.
3204-3214: LGTM: FRect scale_by(2, 4)Relcenter components (w, 2h) are correct; center preserved.
3227-3237: LGTM: FRect scale_by(scale_by=(2, 4))Covers tuple kw path; assertions consistent.
3248-3258: LGTM: FRect scale_by(x=2, y=4)Relcenter and bounds checks are sound.
3266-3276: LGTM: FRect scale_by(0.5, 0.25)Downscale expectations (0.25w, 0.125h) correctly asserted.
3285-3330: LGTM: FRect subzero/negative scaling suiteRobust coverage of sign invariance and axis-specific scaling; relcenter expectations match new half-sizes.
3383-3394: LGTM: FRect scale_by_ip(2.3)In-place behavior mirrors non-inplace; float tolerance applied.
3400-3410: LGTM: FRect scale_by_ip(0.5)Relcenter and bounds verified with float tolerance.
Implementation of #1242 according to MyreMylar name proposal.
While I'm bringing this feature to a potential release, what's your opinion on
rel_centerxandrel_centery?Closes #1242 .
Tests + documentation added.